-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Lens] Add conditional operations in Formula #142325
Conversation
if (Array.isArray(b)) { | ||
return b.map((bi) => { | ||
if (bi === 0) throw new Error('Cannot divide by 0'); | ||
return a / bi; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
divide(4, [1, 0])
would generate an unhandled error without this fix
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
module.exports = { round }; | ||
|
||
function round(a, b = 0) { | ||
function round(a, b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactor here was due to the documentation processor, which handles the default parameter assignment in the function generating a different documentation layout in the functions.md
file for the round
signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could do the defaulting once—
function round(a, _b) {
b = _b ?? 0
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the code here with a cleaner solution in a deeper function.
Returns a value depending on whether the element of condition is true or false. | ||
|
||
Example: Compare two fields average and return the highest | ||
\`ifelse( average(memory) >= average(bytes), average(memory), average(bytes))\` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this example, you can already do it via pick_max
. What about the first example from the "Use cases" section here: #94603 ? Seems understandable to me.
const symbolsToFn = { | ||
'+': 'add', '-': 'subtract', | ||
'*': 'multiply', '/': 'divide', | ||
'<': 'lt', '>': 'gt', '==': 'eq', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we aren't going with =
for equality? As we don't have assignment, it seems like we can go with the simpler case which is also what excel is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Named parameters already use the =
syntax ( i.e. count(kql="...")
).
It should work already, but thought that the ==
operation is something quite familiar as well and different enough to not confuse the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's familiar for developers, I don't think it's super common outside of that group. SQL also doesn't do it. What do you think @ghudgins @ninoslavmiskovic ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also believe that "==" is not known among no developers, and if SQL does not support it, then it is also an indicator that it would be the preferable choice since SQL is a broader query language than KQL. Are there any cases where it will be an issue of sticking with "=" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a quick spike on that to see whether it is possible to co-exists with the named argument.
Unfortunately there are some overlaps between the two which make it harder to solve it, at least at grammar level:
ifelse(a=1, 1, 2) // => 🆘 named argument or comparison? Grammar says it's named argument
ifelse(a=a, 1, 2) // => 🆘 named argument or comparison? Grammar says it's named argument
ifelse(1=1, 1, 2) // => ✅ comparison
ifelse(1=a, 1, 2) // => ✅ comparison
ifelse(unsupported-named-argument=1, 1, 2) // => ✅ comparison
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it should parse correctly, but I didn't check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They all parse fine.
As long as there's no future plan to have string comparison in the future it can work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, in that case I think we should go with the single =
. Otherwise this looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some more investigation on the topic and found out some more issues on the usage for the single =
symbol for comparison.
Here some examples:
- ❌ for plain tinymath it is possible to use a variable for the comparison on
exec
- ❌ in
Canvas
it is possible to use a variable for the comparison:math "ifelse(total_errors = 37, 1, 0)"
which will raise an error about unsupportedNamed parameters
. This is usingexec
underneath. This is probably a non-issue as conditional logic can be performed elsewhere - ❌ If in the future an optimization like [Lens] Improve performance for large formulas #141456 is introduced for comparison, this can be a problem also for Lens in the formula rewrite step.
All of them are minor problems, but still to keep in mind if we decide to go down this single =
symbol route rather than the existing ==
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and decided to keep the ==
symbol.
Some follow ups have been proposed while keeping the double =
:
- add autocomplete on the editor when typing
=
(and magically add another==
). - add operation optimization as [Lens] Improve performance for large formulas #141456 ?
@@ -70,45 +96,55 @@ Variable | |||
|
|||
// expressions | |||
|
|||
// An Expression can be of 3 different types: | |||
// * a Comparison operation, which can contain recursive MathOperations inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty smart to do this within the parsing step, but I worry it's too clever (hard to maintain later on) and it also doesn't create great error messages:
Maybe it makes more sense to pull the type check logic into a separate step after the parsing? Walking the tree and keeping track of the type while recursing should work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is generated at validation time while traversing the AST. That is a validation bug 😓 .
The grammar works fine for the expression ifelse(unique_count(extension, kql='...') == 1, count() > 2)
=> try to paste the peggy
file content in here to see it passing: https://peggyjs.org/online.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I like ifelse
! It doesn't look like we have any camelcase functions to this point, so ifElse
would be a style break.
ifelse( 5 > 6, 1, 0) // returns 0 | ||
ifelse( 1 == 1, [1, 2, 3], 5) // returns [1, 2, 3] | ||
ifelse( 1 < 2, [1, 2, 3], [2, 3, 4]) // returns [1, 2, 3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: leading spaces
module.exports = { round }; | ||
|
||
function round(a, b = 0) { | ||
function round(a, b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could do the defaulting once—
function round(a, _b) {
b = _b ?? 0
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, left two small nits but overall great work!
@@ -253,7 +253,7 @@ describe('math(resp, panel, series)', () => { | |||
)(await mathAgg(resp, panel, series)((results) => results))([]); | |||
} catch (e) { | |||
expect(e.message).toEqual( | |||
'Failed to parse expression. Expected "*", "+", "-", "/", end of input, or whitespace but "(" found.' | |||
'Failed to parse expression. Expected "*", "+", "-", "/", "<", "=", ">", end of input, or whitespace but "(" found.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - should be ==
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is correct. The grammar picks the first possible valid char at the time.
If the expression becomes divide = params.a, params.b
it will error with Expected "=" but " " found
as it's the only valid route.
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/util.ts
Outdated
Show resolved
Hide resolved
…definitions/formula/util.ts
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting changes to x-pack/plugins/security/public/management/roles/edit_role/privileges/kibana/feature_table/feature_table.tsx
LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #94603
lt
,gt
,eq
,lte
,gte
functions.md
pageifelse
operationifelse
name is a placeholder. Possible alternatives are:ifElse
,when
,ifThen
, or make your proposal. :)>
,<
,==
,>=
,<=
grammar.peggy
content into this web site to test the resulting AST: https://peggyjs.org/online.htmlifelse
)ifelse
functionExtra fixes
divide
operation with adivide by 0
scenario.Jest esm error
messageNew grammar changes examples
Tinymath documentation
Command I've used to generate the new
functions.md
file:I couldn't make it work properly the previous command used when tinymath was in its own repo: the produced json for the documentation had the wrong function name, always set to
module.exports
.In the
jsdoc-to-markdown
documentation it mentions thatmodule.exports
had to be defined after the actual function implementation (which is apparently a jsdoc limitation], and after that everything worked properly. That is why some many files changed with only the exports.Demo
Working example:
Comparison symbols and functions working as condition:
Errors:
Documentation:
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers